Conversation
f630964 to
2953725
Compare
1f530e8 to
8faafdf
Compare
apis/validator/block.v3.yaml
Outdated
| oneOf: | ||
| - title: "ProduceBlockV3Response (Gloas)" | ||
| type: object | ||
| required: [version, data, consensus_block_value] |
There was a problem hiding this comment.
I removed the execution_payload_blinded and execution_payload_value from being required in the response post-gloas since there's no longer a concept of blinded blocks, and the block's "value" is now the bid, BeaconBlockBody::SignedExecutionPayloadBid::value
but lmk if these fields should be resurrected
not really sure if we need consensus_block_value at this point. it doesn't seem used in lighthouse, but I kept it as required since perhaps other clients use it
There was a problem hiding this comment.
introducing a produceBlockV4 makes much more sense imo
There was a problem hiding this comment.
The consensus_block_value is used by multi-node validator clients like Vero/Vouch, let's please keep it unless there's a good reason to remove it.
There was a problem hiding this comment.
thanks for the feedback. we agreed that produceBlockV4 makes more sense on the last epbs breakout room call, so I added the endpoint and left produceBlockV3 untouched. produceBlockV4 has consensus_block_value as required per @eth2353's description as to why it's useful
There was a problem hiding this comment.
produceBlockV4 definitely makes more sense
This is not the design for dual deadlines though: there is a single object to sign at the second deadline, but the beacon will return payload present |
| - name: builder_index | ||
| in: path | ||
| required: true | ||
| description: "Index of the builder from which the execution payload envelope is requested." |
There was a problem hiding this comment.
Do we really need the builder here? I assume this is to be signed, but there can only be different builder indices in case of an equivocation or a bad fork. Shouldn't the beacon just return the one for it's head view?
There was a problem hiding this comment.
@potuz, great question. I saw that prysm was passing builder_index in the GetExecutionPayloadEnvelope here
My first thought was that it's a nice early return to prevent expensive state root calculation supposing the passed in builder_index doesn't match the one in the envelope
Possibly credible scenarios I considered:
a. a proposer (self-building) and an in-protocol builder could call on the same beacon node to create bids for a slot. That BN would want to cache both resulting envelopes containing their respective builder_index. Then, after the block is released, either the in-protocol builder or the self-building proposer's bid would be included, so they'd want to fetch their cached envelope from the BN, and would use their builder_index to grab the correct one
b. rare edge cases like chain re-org causing proposer duties to change for a slot, and the VC doesn't remove the previously assigned proposer quickly enough, and you end up with two proposers for the same slot. if both proposers are connected to same BN, you'd want to be able to fetch the right envelope based off their builder_index
There was a problem hiding this comment.
in case the block was build locally do we just pass the validator index here?
apis/validator/block.v3.yaml
Outdated
|
|
||
| Metadata in the response indicates the type of block produced, and the supported types of block | ||
| will be added to as forks progress. | ||
| are for all pre-Gloas forks. This endpoint is not forwards compatible after the Fulu fork. |
There was a problem hiding this comment.
Does it make sense to deprecate produceBlockV3?
There was a problem hiding this comment.
shouldn't need it anymore after the fork transition, right? not sure if it's customary to deprecate now or in separate PR after the fork is live on mainnet. I welcome more thoughts on this
There was a problem hiding this comment.
Exactly, yeah, produceBlockV3 is stated in this PR to not work into Glamsterdam. So if produceBlockV4 will work in pre-Glamsterdam forks, it can and will completely replace produceBlockV3.
Regarding the deprecation, the overall goal per existing custom would be to have deprecated for the duration of Glamsterdam, i.e. there's one hardfork during which people can adjust tooling for their other use cases where in theory produceBlockV3 might still suffice, then by H-fork it can be removed from the API.
I don't know if it's critical exactly when the deprecation would happen to achieve this timeline; there's an argument that if one waits until just after the mainnet fork, well, it's not been deprecated for all of mainnet Glamsterdam, has it, etc. So it should cleanly line up somehow with that fork, or be treated as such for the deprecation-then-one-hardfork-later-remove logistics.
There was a problem hiding this comment.
ah, so produceBlockV4 won't work in pre-glamsterdam forks because it won't support response headers like execution_payload_value or execution_payload_blinded. Additionally, it doesn't support sending back BlockContents containing block, kzg_proofs, and blobs, just block going forward.
originally, I thought backwards compatibility might be preferable, so I had just extended produceBlockV3 to support gloas, but the consensus is to make a new produceBlockV4 as discussed here, so I made the simplified produceBlockV4
is there utility in supporting produceBlockV3 post-gloas? I had looking through lighthouse codebase, and it seems like the produceBlockV3 logic is only called by the VC during block production, so my assumption was we'd need produceBlockV3 and produceBlockV4 to handle fork transition. then, later, we could rip out produceBlockV3. but maybe other clients or otherwise will have some utility of being able to use produceBlockV3 to be able to create pre-gloas blocks?
There was a problem hiding this comment.
Yeah, if produceBlockV4 won't be able to create pre-Glamsterdam blocks, probably not ideal to deprecate produceBlockV3.
There was a problem hiding this comment.
Usually the new api which deprecates the previous one is backward compatible since we wanna keep supporting previous forks, but when it comes to block production, many clients already can't produce pre-electra blocks due to removing eth1 deposit code. I think we can keep v3 for now without deprecating it as doing so would suggest we would remove it in H* which is not clear we wanna do that, also depends on how long we need to support pre-gloas for other networks.
| description: "Slot for which the execution payload bid is requested. Must be current slot or next slot." | ||
| schema: | ||
| $ref: "../../beacon-node-oapi.yaml#/components/schemas/Uint64" | ||
| - name: builder_index |
There was a problem hiding this comment.
apologies in advance, i'm asking this without understanding the full scope of changes in gloas, but the way this API reads sounds like it's requires a beacon node to be configured for builder setup via flag. If that's the case we should specify in the description for getExecutionPayloadBid that some error code should be returned if the bn only allows for self building.
There was a problem hiding this comment.
it is correct that this GET execution_payload_bid endpoint will only need to be called by validator's who register as builders in order to be able to construct a bid
The BN that this builder queries could create a bid in at least a couple of ways:
a. call get_payload and return payload retrieved from it's local EL, basically same flow as self-building
b. use some modified get_payload that retrieves a payload using more advanced sequencing, as discussed a bit on discord
I suppose there would be some configuration flag in the BN to decide whether to retrieve payload via approach a or b
but since the BN could return a payload either way, perhaps we don't need any additional error codes?
There was a problem hiding this comment.
Perhaps an error could be if the builder index is not a 0x03 validator, there's no need to get a bid, even locally, if it's not for an actual builder.
There was a problem hiding this comment.
that makes sense. added here
beacon-APIs/apis/validator/execution_payload_bid.yaml
Lines 67 to 75 in b30273c
ensi321
left a comment
There was a problem hiding this comment.
I think we also need to deprecate/remove all apis related to blinded block. Could be done in another PR
validator-flow.md
Outdated
| This requires registering with builder-specific withdrawal credentials (`BUILDER_WITHDRAWAL_PREFIX`). | ||
|
|
||
| Building: | ||
| 1. [Fetch ExecutionPayloadBid](#/Validator/getExecutionPayloadBid) from beacon node |
There was a problem hiding this comment.
when would we actually ask the beacon node for a bid? currently the bidding for slot N already starts in slot N-1
There was a problem hiding this comment.
@nflaig, I see in the builder section of spec that it says, "Builders can broadcast a payload bid for the current or the next slot's proposer to include."
so I updated this section to:
1. [Fetch ExecutionPayloadBid](#/Validator/getExecutionPayloadBid) from beacon node for the current or next slot's proposer to include.
but that doesn't really answer your question of when the builder technically has to ask the beacon node for a bid as to be included in the block. maybe it makes sense to add something like, "if submitting a bid for the current slot, the bid should be published at the start of the slot". or should be just not be opinionated on when to submit the bid?
cc @potuz
There was a problem hiding this comment.
or should be just not be opinionated on when to submit the bid?
maybe that's the way to go, even though the spec states the following
Validators are still expected to propose
SignedBeaconBlockat the beginning of
any slot
but realistically you are not gonna pick the best bid at t=0 of the slot but rather use the available time you have to collect a better bid, so very likely competitive bids will not even be published at the start of the slot but close to the ATTESTATION_DUE_BPS_GLOAS deadline (currently 3 seconds into the slot)
| - name: builder_index | ||
| in: path | ||
| required: true | ||
| description: "Index of the builder from which the execution payload envelope is requested." |
There was a problem hiding this comment.
in case the block was build locally do we just pass the validator index here?
it's the same as with |
a6624b8 to
1aafd35
Compare
james-prysm
left a comment
There was a problem hiding this comment.
lgtm, glad the proposal path got broken out
The flow for local block building is 1. Create execution payload and bid 2. Construct beacon block 3. Sign beacon block and publish 4. Sign execution payload and publish This PR adds the beacon block v4 flow , GET payload envelope and POST payload envelope (local block building only). The spec for these endpoints can be found here: ethereum/beacon-APIs#552 and is subject to change. We needed a way to store the unsigned execution payload envelope associated to the execution payload bid that was included in the block. I introduced a new cache that stores these unsigned execution payload envelopes. the GET payload envelope queries this cache directly so that a proposer, after publishing a block, can fetch the payload envelope + sign and publish it. I kept payload signing and publishing within the validators block service to keep things simple for now. The idea was to build out a block production MVP for devnet 0, try not to affect any non gloas code paths and build things out in such a way that it will be easy to deprecate pre-gloas code paths later on (for example block production v2 and v3). We will eventually need to track which beacon node was queried for the block so that we can later query it for the payload. But thats not needed for the devnet. Co-Authored-By: Eitan Seri- Levi <eserilev@gmail.com> Co-Authored-By: Michael Sproul <michael@sigmaprime.io> Co-Authored-By: Jimmy Chen <jchen.tc@gmail.com> Co-Authored-By: Eitan Seri-Levi <eserilev@ucsc.edu>
Since the epbs consensus specs are in good shape, we can formalize the beacon api endpoints.
Changes
Updated:
POST /eth/v2/beacon/blocksBeaconBlockto the networkNew:
ExecutionPayloadBid
GET /eth/v1/validator/execution_payload_bid/{slot}/{builder_index}POST /eth/v1/beacon/execution_payload_bidPTC
POST /eth/v1/validator/duties/ptc/{epoch}GET /eth/v1/validator/payload_attestation_data/{slot}POST /eth/v1/beacon/pool/payload_attestationsGET /eth/v1/beacon/pool/payload_attestationsTBD
DataColumnSidecarby whoever's bid is included in the consensus block or perhaps this can be triggered after the call to release aSignedExecutionPayloadEnvelopemay need to later separate theGET /eth/v1/validator/payload_attestation_data/{slot}andPOST /eth/v1/beacon/pool/payload_attestationsinto two endpoints if we end up doing dual-ptc deadlines, which would result in separate payload attestation objects, one forpayload_presentand another forblob_data_available